-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop Usage of ensureGil
from PythonDeephavenSession and Copy Current Scope
#5145
Conversation
engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java
Show resolved
Hide resolved
engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java
Outdated
Show resolved
Hide resolved
engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java
Outdated
Show resolved
Hide resolved
engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java
Outdated
Show resolved
Hide resolved
engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java
Show resolved
Hide resolved
/** | ||
* @return the current scope or the main globals if no scope is set | ||
*/ | ||
PyDictWrapper currentScope(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this in the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we were using getKeysRaw
(through delegation) and getEntriesRaw
both of which created a Stream
around iterating items in the currentScope
. Now we must make a copy/dictCopy prior to using the returned values. (We were assuming that we could ensureGil
while collecting keys and/or values, but this is not the case as performing these operations involve JNI.
I will remove the superfluous methods since I don't believe there is a good way to use them safely anymore.
Cleans up the
toMap
interface for use in other contexts (e.g. they want allTable
instances) and Python copiescurrentScope
instead of trying to do the work under the GIL.Also fixes #5138 by only disallowing
i
,ii
,k
when used as formula parameters - allowing users to name tables in their REPL these things (and observe expected UI behavior).Also fixes #2324 which is the java-reserved-keyword version of #5138.